Skip to content
This repository was archived by the owner on Nov 6, 2023. It is now read-only.

Update Groupon.com.xml#10836

Merged
J0WI merged 11 commits into
masterfrom
unknown repository
Aug 20, 2017
Merged

Update Groupon.com.xml#10836
J0WI merged 11 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Jul 1, 2017

Copy link
Copy Markdown

@cschanaj cschanaj mentioned this pull request Jul 1, 2017
Closed
45 tasks
@Hainish Hainish added the top-1k label Jul 1, 2017
J0WI
J0WI reviewed Jul 6, 2017
View reviewed changes
Comment thread src/chrome/content/rules/Groupon.com.xml Outdated
Nonfunctional subdomains:

- @ ⁴
- www ⁴

@J0WI J0WI Jul 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not confirm the 403 on them. Maybe you have to use a static resource as test url.

@J0WI J0WI closed this Jul 8, 2017
@J0WI J0WI reopened this Jul 8, 2017
@J0WI

J0WI commented Jul 8, 2017

Copy link
Copy Markdown
Contributor

This 403 error is likely because of a bug in our test infrastructure.

@J0WI J0WI closed this Jul 22, 2017
@J0WI J0WI reopened this Jul 22, 2017
J0WI
J0WI reviewed Jul 22, 2017
View reviewed changes
Comment thread src/chrome/content/rules/Groupon.com.xml
<target host="grassroots.groupon.com" />
<target host="jobs.groupon.com" />
<target host="m.groupon.com" />
<target host="merchants.groupon.com" />

@J0WI J0WI Jul 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WFM

@J0WI J0WI Aug 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merchants WFM, but it seems to fail for Travis :(

J0WI
J0WI reviewed Jul 22, 2017
View reviewed changes
Comment thread src/chrome/content/rules/Groupon.com.xml Outdated
@@ -25,18 +18,13 @@ Fetch error: http://touch2.groupon.com/ => https://touch2.groupon.com/: (28, 'Co

Fully covered subdomains:

@J0WI J0WI Jul 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed anyway, because all targets ar listed below.

@J0WI

J0WI commented Aug 6, 2017

Copy link
Copy Markdown
Contributor

ping @koops76

@cschanaj

cschanaj commented Aug 17, 2017

Copy link
Copy Markdown
Collaborator

@koops76 do you mind to update this PR for completion of #10782?

@ghost

ghost commented Aug 17, 2017

Copy link
Copy Markdown
Author

@cschanaj @J0WI What should I change?

@J0WI

J0WI commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

See #10836 (comment) and #10836 (comment)

@ghost

ghost commented Aug 17, 2017

Copy link
Copy Markdown
Author

@J0WI Done.

@ghost

ghost commented Aug 18, 2017

Copy link
Copy Markdown
Author

@J0WI Whitelist I guess?

@J0WI

J0WI commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

Let's try a static ressource first:
http://merchants.groupon.com/assets/main_app/background.png

@ghost

ghost commented Aug 18, 2017

Copy link
Copy Markdown
Author

@J0WI m still fails the test.

@J0WI

J0WI commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

Yes, my comment was only for merchants. Sorry for the confusion.

@J0WI

J0WI commented Aug 20, 2017

Copy link
Copy Markdown
Contributor

Let's try a static ressource first:
http://merchants.groupon.com/assets/main_app/background.png

Was this not successful?

@ghost

ghost commented Aug 20, 2017

Copy link
Copy Markdown
Author

@J0WI It was, but m is still failing the test.

@J0WI

J0WI commented Aug 20, 2017

Copy link
Copy Markdown
Contributor

Yes, that's expected since m times out. But you also re-removed merchants.
I was asking you to try a static resource on merchants and otherwise we have to white listing it.

@ghost

ghost commented Aug 20, 2017

Copy link
Copy Markdown
Author

@J0WI Done.

@J0WI

J0WI commented Aug 20, 2017

Copy link
Copy Markdown
Contributor

Okay, this really requires to be whitelisted.

@ghost

ghost commented Aug 20, 2017

Copy link
Copy Markdown
Author

@J0WI Done.

@J0WI

J0WI commented Aug 20, 2017

Copy link
Copy Markdown
Contributor

Thanks!
Please avoid merge commits in PRs.

@J0WI J0WI merged commit 828c815 into EFForg:master Aug 20, 2017
@ghost ghost deleted the patch-28 branch August 20, 2017 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

top-1k

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@J0WI @cschanaj @Hainish